Cache resolved LLM provider credentials per agent build#1947
Conversation
build_agent_model -> _get_db_credentials ran on every agent build (every chat message, structured-output call, and memory-curation task) and resolved the provider secret through PipelineSettings.get_full_component_settings, which reads the encrypted secret store twice and so ran the deliberately expensive PBKDF2 KDF (480k HMAC iterations) twice per build whenever any provider key was configured. Memoize the resolved per-provider credentials in-process keyed on (class_path, PipelineSettings.modified) -- the same key the reranker/embedder instance caches use. Repeat builds skip the Fernet/PBKDF2 decryption while live rotation is preserved: a superuser key change calls PipelineSettings.save(), which bumps modified (auto_now) and clears the singleton cache, so the next build misses the memo and re-decrypts (no redeploy; no staleness beyond the existing 5-minute PipelineSettings cache TTL). DB-wins / env-fallback precedence is intact. invalidate_credential_cache() mirrors invalidate_embedder_cache / invalidate_reranker_cache and is wired into conftest for test isolation; call it after any out-of-band singleton write that bypasses save(). Closes #1921
Code ReviewOverviewThis PR memoizes resolved LLM provider credentials in Strengths
IssuesMinor —
Minor —
Low — stale entry accumulation between explicit invalidations Each Nit# model_factory.py
_CREDENTIAL_CACHE: dict[tuple[str, Any], dict[str, str]] = {}
# Guards against two concurrent builds paying the decryption cost twice.
_CREDENTIAL_CACHE_LOCK = threading.Lock()"two concurrent builds" → more precisely "concurrent builds on the same key". The lock serialises any number of threads racing on a cold entry, not just two. Minor phrasing nit; the intent is clear from context. SummaryThe implementation is correct, follows the established caching pattern faithfully, and the tests cover the scenarios that matter (memoization, live rotation, explicit purge). No blocking issues. The three items above are all low-priority and two of them (conftest alias, stale-entry note) are pre-existing or cosmetic. Good to merge after a quick look at whether the inline cache-key logic deserves a comment. |
2 participants
What & why
Closes #1921.
opencontractserver/llms/model_factory.py::_get_db_credentialsruns on every agent build — every chat message, every structured-output call, every memory-curation task (and the corpus-logo path viaaget_provider_credentials). It resolves the provider's secret throughPipelineSettings.get_full_component_settings, which reads the encrypted secret store twice (merged settings + secrets overlay). Each read decrypts the Fernet blob, and that decryption derives its key with the deliberately expensive PBKDF2 KDF (PIPELINE_SETTINGS_ENCRYPTION_ITERATIONS, default 480 000 HMAC iterations — seePipelineSettings._derive_key). So once any provider key was configured, two PBKDF2 passes ran on every single build.PipelineSettings.get_instance()is already cache-backed, so this was never a per-call DB round-trip, but the repeated decryption is pure wasted work.How
Memoize the resolved per-provider credentials in-process, keyed on
(class_path, PipelineSettings.modified)— the same cache key the reranker and embedder instance caches already use (opencontractserver/pipeline/utils.py). This is the codebase's established pattern for "expensive-to-resolve, lives on thePipelineSettingssingleton, must stay live-configurable."threading.Lockwith the standard double-checked-locking guard so concurrent builds decrypt at most once. Returned dicts are copies, so a caller can never poison the cache.PipelineSettings.save(), which bumpsmodified(auto_now) and clears the singleton cache. The next build misses the memo on the new key and re-decrypts — a rotated/cleared key takes effect with no redeploy, and the memo adds no staleness beyondget_instance()'s own 5-minute TTL. DB-wins / env-fallback precedence is unchanged.invalidate_credential_cache()mirrorsinvalidate_embedder_cache/invalidate_reranker_cache; it's wired intoconftest.pyfor per-test isolation and is the escape hatch for any out-of-band singleton write that bypassessave()(QuerySet.update,bulk_update, raw migrations — the same issue Extract Pipeline Cleanup #1410 caveat the sibling caches carry).This implements the issue's first suggested direction (memoization keyed on
modified) rather than restructuring the fivemake_pydantic_ai_agentcall sites, so it covers every credential-resolution path — chat and image generation — at the single chokepoint, with no agent-construction churn.Tests
opencontractserver/tests/test_llm_model_factory.py::TestCredentialCache:test_resolution_memoized_across_builds— two builds, secret store decrypted once (get_secretscalled exactly twice = oneget_full_component_settings).test_rotated_key_takes_effect_without_redeploy— rotate viasave()mid-test; the second build picks up the new key (regression guard for the Register LLM providers as DB-singleton pipeline components with live credentials & endpoints #1897 live-config guarantee).test_invalidate_credential_cache_forces_redecrypt— explicit purge re-decrypts (out-of-band path).black,isort(profile black), andflake8are clean on the changed files; the changelog fragment passesscripts/collate_changelog.py --check. The Dockerized backend suite (which imports pydantic-ai) couldn't run in this environment, but the cache control-flow (memoization, copy-on-return, rotation invalidation, explicit invalidation, empty-creds caching, 16-thread single-decrypt) and theautospec+side_effectspy semantics the assertions rely on were both validated with standalone harnesses.Notes
PipelineSettingsTTL, exactly like the reranker/embedder caches.docs/architecture/llms/README.mdAPI-keys section gains a paragraph on the memo + invalidation contract.Generated by Claude Code